Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Globe view marker support #11556

Merged
merged 30 commits into from
Mar 29, 2022
Merged

Globe view marker support #11556

merged 30 commits into from
Mar 29, 2022

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Feb 25, 2022

Closes https://github.com/mapbox/gl-js-team/issues/373

Default viewport alignment (pitchAlignment: viewport & rotationAlignment: viewport):

marker-viewport-viewport.mov

Map alignment(pitchAlignment: map & rotationAlignment: map):

marker-map-map-2.mov

Two small issues with the current approach:

  • During the globe to Mercator transition, markers are placed as if the map was still a full globe, resulting in incorrect placement and a jump in position when the map crosses the threshold into pure Mercator at high pitch. Fixing this issue would require changing the behavior of Globe.locationPoint to account for the transition, which probably would amount to a change in the globe matrix calculation. This change should also improve the behavior of various API calls during the transition. This fix is out of scope for this PR, but the benefits of a more accurate transformation matrix mean that this is worth investigating. Alternatively, these issues could also be minimized by moving the transition to a higher zoom.

  • With map alignment, the marker's angle can appear slightly incorrect, particularly when a marker is near to or on the horizon. This could be resolved in the future by a more accurate marker placement model that transforms the marker according to its 3d position on the globe (as opposed to the current approximation handling x + y rotations separately from z rotations).

Both of these issues are subtle enough that I don't see them as blockers to this PR.

Launch Checklist

  • Fix marker placement still in Mercator
  • Hide markers behind globe
  • Disable marker interactions when hidden
  • Orient markers away from center of globe
  • Improve behavior with pitched map
  • Transition to Mercator
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

@mapbox/map-design-team

@SnailBones SnailBones marked this pull request as draft February 25, 2022 16:43
@SnailBones SnailBones added 3d 📐 skip changelog Used for PRs that do not need a changelog entry labels Feb 28, 2022
src/ui/map.js Outdated Show resolved Hide resolved
@SnailBones SnailBones mentioned this pull request Mar 4, 2022
3 tasks
@karimnaaji karimnaaji mentioned this pull request Mar 11, 2022
@SnailBones SnailBones requested a review from karimnaaji March 23, 2022 16:43
@SnailBones SnailBones marked this pull request as ready for review March 23, 2022 16:43
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A first pass of review! This looks great so far.

src/geo/projection/globe_util.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
src/ui/marker.js Outdated Show resolved Hide resolved
@SnailBones SnailBones requested a review from karimnaaji March 28, 2022 15:43
src/geo/transform.js Outdated Show resolved Hide resolved
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more pass of review with the following notes:

  • I suggest to disable occlusion for markers to remove the extraneous 100km check concerning Globe view marker support #11556 (comment)
  • I think we can cleanup the logic around locationPoint3D API to remove the extraneous globe branching: Globe view marker support #11556 (comment)
  • Question: Do you know what could be the reason of this slight precision difference when crossing the threshold mercator/globe? The difference is large enough that it could be pointing at an incorrect location. I also wonder if that's the limitation you mentioned in the description around the transition support:
marker-jump.mov
marker-jump2.mov

@SnailBones
Copy link
Contributor Author

SnailBones commented Mar 29, 2022

Thanks for the review @karimnaaji!

I'm not sure I understand what you're suggesting here. Occlusion on globe is an intended feature that I've added to this PR, and disabling it makes markers visible on the far side of the globe.

The 100km check could be removed with a different occlusion algorithm, but my sense is that to keep code simple and performant--especially if we need to support 3d terrain on globe in the future--we should strive to use a consistent approach across terrain and globe.

EDIT: Thinking more about this, we want to hide markers behind the globe while only making ones hidden behind terrain transparent. So it actually makes sense to handle each in turn, and it's worth investigating other methods to improve performance, accuracy, or simplify the code of globe occlusion.

  • Question: Do you know what could be the reason of this slight precision difference when crossing the threshold mercator/globe? The difference is large enough that it could be pointing at an incorrect location. I also wonder if that's the limitation you mentioned in the description around the transition support:

Yes, that's the issue I mention in the description. I share some ideas on how to solve the issue there, IMO the solution is out of scope for this PR.

Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice work @SnailBones .

Let's follow up on simplifying occlusion checks for globe, as it should end up to only looking at the dot product sign between the camera forward direction and center-marker location vectors, similarly to how it's done here. This would simplify those checks and remove the extraneous 100kms padding introduced due to the lack of accuracy of the method, which in that case is well suited for terrain and not as much for globe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3d 📐 skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants